fix for console log URL #13

hrishin · 2018-02-09T07:39:51Z

With introduction of Jenkins ideler/proxy have to fix the console URL generation
With that higher precedence is given to ENV VAR having proxy URL, otherwise, URL of Jenkins service if it does exist in OpenShift
Increased poll timing from 1 seconds to 10 seconds. As it could flood too many API requests(GET/PATCH) to OpenShit API server

hrishin · 2018-02-09T07:56:41Z

This is how URL looks like http://www.awesomescreenshot.com/image/3162147/19df19d3a5cfe6a0c3d5a66e49698be8 in OpenShift console

Note: URL host coming from ENV VAR

CC: @hferentschik @vpavlin

hferentschik · 2018-02-09T10:26:12Z

src/main/java/io/fabric8/jenkins/openshiftsync/BuildSyncRunListener.java

@@ -162,7 +164,7 @@ protected void doRun() throws Exception {
          pollLoop();
        }
      };
-      Timer.get().scheduleAtFixedRate(task, pollPeriodMs, pollPeriodMs, TimeUnit.MILLISECONDS);
+      Timer.get().scheduleAtFixedRate(task, pollPeriodDelayMs, pollPeriodMs, TimeUnit.MILLISECONDS);


How is this related to the issue?

It's indirectly related. Recently similar issue has been observed in plugin behavior and the root cause of this issue was the frequency at which it polls the OpenShift API's.
IMHO, it would be better if this fix can go along with this issue. It may sound premature optimization though and not directly related.

In this case, I'd recommend putting this into a dedicated commit with a proper commit message. Nothing stops you to address two issues within the same pull request, but for traceability, it makes sense to separate this.

IMO you should create an issue for this in this project referencing openshiftio/openshift.io#1648. Then you split this commit into to dedicated commits, each referencing its matching issue.

makes sense

IMHO, it would be better if this fix can go along with this issue.

Why?

I mean, I understand why we want this change in, but why does it have to go "along with this issue"?

In any case, please split the commits as Hardy suggested.

hferentschik · 2018-02-09T10:30:18Z

No test? What's about referencing the issue in the commit message?

rupalibehera · 2018-02-09T14:48:14Z

@hrishin the PR looks good to me, let me me know when you are happy with it we can merge it.

hferentschik

Please create dedicated issues and split commits. And what's about testing?

hferentschik · 2018-02-09T18:21:07Z

That said, feel free to ignore the review. It's not my call in the end.

hrishin · 2018-02-12T13:16:10Z

@hferentschik reverted polling changes. @rupalibehera can we merge it and test it if it looks good?

hferentschik

@hrishin would you mind rebasing and squashing this? There no need for three commits and you literally want to remove that this polling change was ever in there.

Other than that, it looks ok. As I said, I recommend starting to adopt some issue referencing scheme in the commit messages, but you might want to discuss within the team first what you want to adopt.

Awesome with tests, btw.

hferentschik · 2018-02-12T13:41:36Z

src/test/java/io/fabric8/jenkins/openshiftsync/BuildSyncRunListenerTest.java

+  public final org.junit.contrib.java.lang.system.EnvironmentVariables environmentVariables = new org.junit.contrib.java.lang.system.EnvironmentVariables();
+
+  @Test
+  public void should_get_hostname_from_env_var() {


hferentschik · 2018-02-12T13:41:48Z

src/test/java/io/fabric8/jenkins/openshiftsync/BuildSyncRunListenerTest.java

+  }
+
+  @Test
+  public void should_get_hostname_from_openshift_service() {


hferentschik · 2018-02-12T13:42:15Z

src/test/java/io/fabric8/jenkins/openshiftsync/BuildSyncRunListenerTest.java

+  }
+
+
+}


There is a newline missing. Does not matter for Java, but it would keep GitHub/git happy.

hferentschik · 2018-02-12T14:35:14Z

If you need help with the rebase, let me know. Happy to help.

rupalibehera · 2018-02-12T15:28:29Z

@hferentschik , I can rebase and merge while merging this PR from the github UI

hferentschik · 2018-02-12T15:34:35Z

I can rebase and merge while merging this PR from the GitHub UI

Sure, but @hrishin might want to adjust the commit messages or something. There is a difference in forcing a squash and doing a proper rebase of a pull request.

rupalibehera · 2018-02-12T15:38:16Z

it is late in India now, I am building the image with this fork and test if things work and then we can merge this one.

rupalibehera · 2018-02-12T15:58:56Z

@hrishin , tests are failing while compiling using mvn clean install below are the logs :
[INFO] Running InjectedTest [ERROR] Tests run: 11, Failures: 0, Errors: 5, Skipped: 0, Time elapsed: 12.432 s <<< FAILURE! - in InjectedTest [ERROR] testPluginActive(org.jvnet.hudson.test.PluginAutomaticTestBuilder$OtherTests) Time elapsed: 0.006 s <<< ERROR! java.lang.Error: Plugin cloudbees-folder failed to start at org.jvnet.hudson.test.PluginAutomaticTestBuilder$OtherTests.testPluginActive(PluginAutomaticTestBuilder.java:99) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
@hrishin , let me know if I am missing something here

gabemontero · 2018-02-12T17:30:45Z

src/main/java/io/fabric8/jenkins/openshiftsync/BuildSyncRunListener.java

@@ -258,7 +259,7 @@ private void upsertBuild(Run run, RunExt wfRunExt) {
    }

    OpenShiftClient openShiftClient = getOpenShiftClient();
-    String rootUrl = OpenShiftUtils.getJenkinsURL(openShiftClient, namespace);
+    String rootUrl = getHostName(openShiftClient, namespace);


As I mentioned in PR 12, for a bugzilla we just got from a multi namespace customer, I will be making a change to use the jenkins pod namespace before the build namespace for this ... I'll tag you all in that PR.

I believe that should be fine with your env var override based on how this is constructed

@gabemontero 👍

cool ... for cross referencing, here is the upstream pr: openshift#198

hrishin · 2018-02-13T06:04:24Z

@rupalibehera

tests are failing while compiling using mvn clean install below are the logs

I'm sorry that's my bad :(, missed something to make it work. Now fixed the plugin loading issue to run the tests. Let's try to build an image and test it.

@hferentschik
yeah rebased and squashed. 👍

hferentschik

Ok, I added some more comments. The docs are not quite right I think. AFAIU we are configuring a URL not a hostname. Two different things.

Could you also please re-craft the commit message. You are mixing subject and body. This does not look and read well. See for example https://chris.beams.io/posts/git-commit/. You want to have a subject line, followed by an empty line, followed by the actual change description which you already have.

hferentschik · 2018-02-13T08:02:15Z

src/test/java/io/fabric8/jenkins/openshiftsync/BuildSyncRunListenerTest.java

+    openShiftServer.expect().get().withPath("/oapi/v1/namespaces/default/routes").andReturn(200, routeList).always();
+    return openShiftServer;
+  }
+}


Still missing a newline

but wondering why its so important?

This is not really important. It does not really matter, especially for Java. However, in some cases (languages, config file types) it does matters and hence GitHub flags it. Best practice is to have the newline.

hferentschik · 2018-02-13T08:03:39Z

README.md

+Configuration
+------------------------
+Jenkins Build Log URL: 
+* Plugin adds a annotation in Build resource metadata which contains URL to access Jenkins build logs.


The plugin, instead of just plugin

an annotation

hferentschik · 2018-02-13T08:07:48Z

README.md

+------------------------
+Jenkins Build Log URL: 
+* Plugin adds a annotation in Build resource metadata which contains URL to access Jenkins build logs.
+By default host name for log URL comes from service running in OpenShift knows as "Jenkins". To override and configure this URL's host name, add an environment variable `JENKINS_ROOT_URL`. 


By default host name for log URL -> By default, the Jenkins base URL for the build log is determined via the OpenShift route of the jenkins service. To override and configure this base URL, you can set the environment variable JENKINS_ROOT_URL.

host name != (base) URL. What are we setting here? I take it is the base URL, right? Then please don't mix this up with hostname. These are two completely different things.

+1 for the docs, just needs some rework

yes its base URL

hferentschik · 2018-02-13T08:13:19Z

@hrishin @rupalibehera - so you don't have a CI building and testing pull requests? Why not?

hrishin · 2018-02-13T11:09:55Z

so you don't have a CI building and testing pull requests? Why not?

@hferentschik its kind of pitta :( to say this but unfortunately, there are no tests in place. Mostly we are planning to get rid of this fork so not sure of how to prioritize it.
It's worth discussing. Will log an issue for that

cc: @rupalibehera

hferentschik · 2018-02-13T11:26:16Z

its kind of pitta :( to say this but unfortunately, there are no tests in place.

What do you mean? There are at the very least your tests, right? And it is not only about running tests, but also knowing that the project simply compiles.

Mostly we are planning to get rid of this fork so not sure of how to prioritize it.

Is this fork used right now? Do you have to cut some more releases of it? If so, it is imo a no-brainer. We are talking about adding a single YAML file and you get for example a Travis CI up and running. Time effort less than an hour.

Create the issue, create a pull request with the Travis config, review, merge, done.

hferentschik · 2018-02-13T11:28:15Z

https://docs.travis-ci.com/user/languages/java/

(note, I don't think this should be part of this pull request. Just saying it is trivial to enable and adds value with hardly any effort)

rupalibehera · 2018-02-13T15:28:43Z

I pulled in @hrishin code from his fork and created the .hpi and added it here https://github.com/rupalibehera/openshift-jenkins-s2i-config/tree/sync-plugin-idler/plugins and the Jenkins image is created a PR here fabric8io/openshift-jenkins-s2i-config#143, I am looking into the test failure.
I will update the my tenant to test this image fabric8/jenkins-openshift:SNAPSHOT-PR-143-2

hferentschik · 2018-02-13T16:03:52Z

src/test/java/io/fabric8/jenkins/openshiftsync/BuildSyncRunListenerTest.java

@@ -0,0 +1,81 @@
+/**
+ * Copyright (C) 2016 Red Hat, Inc.


This is the wrong copyright yea

hferentschik · 2018-02-13T16:06:16Z

README.md

+Jenkins Build Log URL: 
+* The plugin adds an annotation in Build resource metadata which contains URL to access Jenkins build logs.
+By default, the Jenkins base URL for the build log is determined via the OpenShift route of the jenkins service. To override and configure this base URL, you can set the environment variable `JENKINS_ROOT_URL`. 
+This environment variable will get higher precedence than Jenkins service to determine base URL.


"higher precedence" is a misnomer. Precedence already implies the order and that it has a higher rank in this order.

hferentschik · 2018-02-13T16:09:12Z

README.md

+Configuration
+------------------------
+Jenkins Build Log URL: 
+* The plugin adds an annotation in Build resource metadata which contains URL to access Jenkins build logs.


This plugin adds an annotation to the OpenShift build configuration containing the Jenkins build log URL.

hferentschik

The docs are still not quite right, but let's get this merged. We should strive to be concise when talking about the various technical aspects.

hferentschik · 2018-02-13T16:16:55Z

I pulled in @hrishin code from his fork and created the .hpi and added it here https://github.com/rupalibehera/openshift-jenkins-s2i-config/tree/sync-plugin-idler/plugins and the Jenkins image is created a PR here fabric8io/openshift-jenkins-s2i-config#143,

any plans to address the versioning? Can you not add a version string as well? How are the plugins picked up? By name or does it just load all plugins in the directory in which case one could just add the version.

I am looking into the test failure.

You mean the test failure in the openshift-jenkins-s2i-config PR build? TBH, I have a hard time interpreting that it means.

I will update the my tenant to test this image fabric8/jenkins-openshift:SNAPSHOT-PR-143-2

ok, let us know how it goes

hferentschik · 2018-02-13T19:51:44Z

You even have already a Travis config - https://github.com/fabric8io/jenkins-sync-plugin/blob/master/.travis.yml. It's just a matter of checking if it still runs the right target and making sure that pull requests are getting built.

1. With introduction of Jenkins ideler/proxy have to fix the console URL generation With that higher precedence is given to ENV VAR (JENKINS_ROOT_URL) having proxy URL, otherwise, URL of Jenkins service if it does exist in OpenShift. This issue is fixed in BuildSyncRunListener.java 2. Added the unit test for this fix : BuildSyncRunListenerTest.java 3. Fixed test execution issue : Upgraded <artifactId>credentials</artifactId> dependency version 4. Updated README.md with this URL configuration section

hferentschik · 2018-02-14T18:38:37Z

@rupalibehera what's the outcome, did all go well? Could you test and verify this patch?

rupalibehera · 2018-02-14T18:58:04Z

@hferentschik , sorry forgot to update it here the testing of the image went well and everything from the Jenkins perspective looks good, but we were trying to figure out the fix for fabric8-services/fabric8-tenant#528, I believe that is fixed by @chmouel https://github.com/fabric8-services/fabric8-tenant/pull/529/files and fabric8-services/fabric8-tenant-jenkins#65

rupalibehera · 2018-02-14T19:04:02Z

@hrishin @hferentschik , I am merging this PR as testing went well I will update the image with this binary and update the PR fabric8io/openshift-jenkins-s2i-config#143

hrishin requested a review from rupalibehera February 9, 2018 07:40

hrishin force-pushed the job-to-bc branch 2 times, most recently from 6ba872f to e91deb6 Compare February 9, 2018 07:44

hferentschik reviewed Feb 9, 2018

View reviewed changes

hferentschik suggested changes Feb 9, 2018

View reviewed changes

hrishin mentioned this pull request Feb 12, 2018

Jenkins Log URL in OpenShift Console #11

Closed

hferentschik suggested changes Feb 12, 2018

View reviewed changes

gabemontero approved these changes Feb 12, 2018

View reviewed changes

hrishin force-pushed the job-to-bc branch from ec93ea4 to facce2c Compare February 13, 2018 05:57

hrishin force-pushed the job-to-bc branch 4 times, most recently from ab36c06 to a291aff Compare February 13, 2018 08:01

hferentschik suggested changes Feb 13, 2018

View reviewed changes

hrishin force-pushed the job-to-bc branch from a291aff to 494ce89 Compare February 13, 2018 13:04

hrishin force-pushed the job-to-bc branch from 494ce89 to ceb87b4 Compare February 13, 2018 13:07

hferentschik reviewed Feb 13, 2018

View reviewed changes

hferentschik approved these changes Feb 13, 2018

View reviewed changes

gabemontero mentioned this pull request Feb 13, 2018

Fix jenkins namespace for root url openshift/jenkins-sync-plugin#198

Merged

hrishin force-pushed the job-to-bc branch from ceb87b4 to 5b3cfcd Compare February 14, 2018 09:30

hrishin force-pushed the job-to-bc branch from 5b3cfcd to ca7ae78 Compare February 14, 2018 09:36

rupalibehera merged commit 3f9692b into fabric8io:job-to-bc Feb 14, 2018

fix for console log URL #13

fix for console log URL #13

Conversation

hrishin commented Feb 9, 2018 • edited Loading

hrishin commented Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hferentschik commented Feb 9, 2018

rupalibehera commented Feb 9, 2018

hferentschik left a comment • edited Loading

Choose a reason for hiding this comment

hferentschik commented Feb 9, 2018

hrishin commented Feb 12, 2018

hferentschik left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hferentschik commented Feb 12, 2018

rupalibehera commented Feb 12, 2018

hferentschik commented Feb 12, 2018

rupalibehera commented Feb 12, 2018

rupalibehera commented Feb 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrishin commented Feb 13, 2018

hferentschik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hferentschik commented Feb 13, 2018

hrishin commented Feb 13, 2018 • edited Loading

hferentschik commented Feb 13, 2018

hferentschik commented Feb 13, 2018

rupalibehera commented Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hferentschik left a comment

Choose a reason for hiding this comment

hferentschik commented Feb 13, 2018

hferentschik commented Feb 13, 2018

hferentschik commented Feb 14, 2018

rupalibehera commented Feb 14, 2018

rupalibehera commented Feb 14, 2018

hrishin commented Feb 9, 2018 •

edited

Loading

hrishin commented Feb 9, 2018 •

edited

Loading

hferentschik left a comment •

edited

Loading

hferentschik left a comment •

edited

Loading

hrishin commented Feb 13, 2018 •

edited

Loading

rupalibehera commented Feb 13, 2018 •

edited

Loading